Skip to content

Nikita settings page #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nikitalokhmachev-ai merged 10 commits into from
May 30, 2025
Merged

Nikita settings page #38

nikitalokhmachev-ai merged 10 commits into from
May 30, 2025

Conversation

@nikitalokhmachev-ai
Copy link
Collaborator

@nikitalokhmachev-ai nikitalokhmachev-ai commented May 27, 2025 •
edited
Loading

User description

Here's what I've done in this PR:
#35

  • Tackled the bug tied to the "Resume Archiving" button not switching back to Enabled.

#29

  • Added an "Archive Screenshots" setting
  • Added an "Archive Cookies" setting
  • Added an "Archive LocalStorage" setting
  • Implemented the "Domains to skip" feature (this includes subdomains and embeds)

Not sure whether we should pre-populate the "Domains to skip" field and other settings.

PR Type

Bug fix, Enhancement

Description

  • Fix page archivable status on reload

  • Add settings page with archiving options

  • Implement domains to skip feature

  • Fix quick actions after leaving settings

Changes walkthrough 📝

Relevant files
Enhancement
bg.ts
Implement domain filtering and settings sync                         

src/ext/bg.ts

  • Added skip domains functionality to filter URLs
  • Updated isValidUrl to check against skipDomainsSet
  • Added message handler for optionsChanged
  • Improved domain filtering in startRecording
    		• +61/-9   
    recorder.ts
    Add domain filtering to recorder                                                 

    src/recorder.ts

  • Added skipDomains property to Recorder class
  • Implemented domain filtering in commitPage and commitResource
  • Updated initOpts to load skipDomains from local storage
    		• +32/-0   
    settings-page.ts
    Create settings page component                                                     

    src/settings-page.ts

  • Created new settings page component with Material Design elements
  • Implemented toggles for Archive Screenshots, Cookies, and LocalStorage
  • Added text field for domains to skip with persistence
  • Added event handlers to update settings in local storage
    		• +223/-0 
    Bug fix
    sidepanel.ts
    Fix archivable status and integrate settings                         

    src/sidepanel.ts

  • Fixed page archivable status updating on reload
  • Added settings page integration and toggle
  • Removed unnecessary disabled state for Resume Archiving button
  • Added skipDomains filtering to canRecord check
    		• +52/-14 
    Styling
    sidepanel.html
    Add styling for Material Design switch                                     

    static/sidepanel.html

  • Added CSS variables for Material Design switch component
  • Customized switch colors to match application theme
    		• +8/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @pr-agent-monadical
    Copy link

    pr-agent-monadical bot commented May 27, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    35 - Fully compliant

    Compliant requirements:

    • Fix the issue where the "start archiving" button remains disabled when navigating from an unarchivable page to an archivable one
    • The extension should update the page archivable status on reload without requiring the user to close and reopen the extension

    29 - PR Code Verified

    Compliant requirements:

    • Add "Archive Cookies" setting
    • Add "Archive Localstorage" setting
    • Add "Archive screenshots" setting
    • Add "Archiving blocklist" as a text field for URLs (implemented as "Domains to skip")

    Requires further human verification:

    • Verify that "Archive screenshots" is off by default as required

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The PR adds settings to archive cookies and local storage, which could expose sensitive user information. While the UI does include warnings about this risk, the implementation should be carefully reviewed to ensure that users are fully aware of the implications and that the archived data is properly secured. Additionally, the code should be reviewed to verify that the settings are properly enforced and that sensitive data is not inadvertently archived when these settings are disabled.

    ⚡ Recommended focus areas for review

    Potential Security Risk

    The settings page contains strong warnings about security risks when archiving local storage, but the implementation should be reviewed to ensure these settings are properly enforced and that users understand the implications.

     
    <p class="section-desc md-typescale-body-small">
  Archiving local storage will archive information that is generally
  always private.
  <br /><br />
  <strong>Sharing content created with this setting enabled may compromise your
  login credentials.</strong>
</p>
    Duplicate Skip Logic

    The domain skipping logic is implemented in multiple places (bg.ts, recorder.ts, sidepanel.ts) with similar but not identical code, which could lead to inconsistent behavior.

     
    commitPage(currPage, domSnapshot, finished) {
  try {
    const host = new URL(currPage.url).hostname.toLowerCase();
    console.log("Current host: " + host);
    if (
      Array.from(this.skipDomains).some(
        (domain) => host === domain || host.endsWith(`.${domain}`),
      )
    ) {
      console.log("Skipping: " + host);
      return;
    }
  } catch (_) {}
    State Management

    The recording state management has been modified with additional state updates that might cause unexpected behavior if not properly synchronized with the background process.

     
    //@ts-expect-error - TS2339 - Property 'recording' does not exist on type 'ArgoViewer'.
this.recording = true;

    pr-agent-monadical[bot]
    pr-agent-monadical bot reviewed May 27, 2025
    View reviewed changes
    Shrinks99
    Shrinks99 reviewed May 27, 2025
    View reviewed changes
    Copy link
    Collaborator

    @Shrinks99 Shrinks99 left a comment •
    edited
    Loading

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Now that the button doesn't have a disabled state, the status section doesn't seem to be working as expected on un-archivable pages. Also when I turn the extension on while viewing an unarchivable page and navigate to an archivable one it does not start archiving for me.

    Screenshot 2025-05-27 at 3 41 07 PM

    Shrinks99
    Shrinks99 requested changes May 27, 2025
    View reviewed changes
    Copy link
    Collaborator

    @Shrinks99 Shrinks99 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Hmm actually I can't get it to start archiving at all? Seems to be a bug.

    @Shrinks99 Shrinks99 self-requested a review May 29, 2025 06:57
    Shrinks99
    Shrinks99 reviewed May 29, 2025
    View reviewed changes
    Copy link
    Collaborator

    @Shrinks99 Shrinks99 left a comment •
    edited
    Loading

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Things seem to be working! Looking much better. 3 things to address from me and then should be good to go!

    1. I blocked youtube.com which worked (nothing was added to the archive!), but we still tell the user that the page is being archived. Could we add a state for blocked pages that tells the user the page is being blocked due to the blocklist?

    2. The share current page icon appears grey and should be white Fixed!

    3. URLs can overflow if they don't have a nice point to break at. Need to add word-wrap: break-word; I think Fixed!

    Screenshot 2025-05-29 at 12 14 47 PM

    @nikitalokhmachev-ai nikitalokhmachev-ai merged commit 3c227f1 into main May 30, 2025
    0 of 3 checks passed
    @Shrinks99 Shrinks99 deleted the nikita-settings-page branch May 30, 2025 16:14
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @Shrinks99 Shrinks99 Shrinks99 approved these changes

    @yamijuan yamijuan yamijuan approved these changes

    +1 more reviewer

    @pr-agent-monadical pr-agent-monadical[bot] pr-agent-monadical[bot] left review comments

    Reviewers whose approvals may not affect merge requirements

    Assignees

    No one assigned

    Labels

    Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @nikitalokhmachev-ai @Shrinks99 @yamijuan